fix: key instance states by state.value, not state.id (#624)#625
Conversation
PR fgmacedo#592 introduced ``Configuration._instance_states`` as a perf optimization but keyed it by ``state.id`` — the Python attribute name, which is not unique across sibling Compounds. When two ``State.Compound`` regions declared inner states under the same attribute name (e.g. ``g1.a`` and ``g2.a``), the second overwrote the first in the dict, causing dispatch to resolve against the wrong active state and raise ``TransitionNotAllowed``. Re-key ``_instance_states`` by ``state.value`` — the canonical identifier already used as the ``states_map`` key and guaranteed unique by construction when users provide explicit ``value=``. This restores the 3.0.0 resolution path that PR fgmacedo#592 inadvertently changed. Charts that rely on default ``value=`` and reuse Python attribute names across sibling Compounds were never supported (silent ``states_map`` overwrite at class-build time, predates fgmacedo#592). Surfacing that case as ``InvalidDefinition`` is tracked as a separate follow-up. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Andrey Nenashev <anenashev90@gmail.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #625 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 42 42
Lines 4976 4976
Branches 812 812
=========================================
Hits 4976 4976
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @AndreyNenashev, thanks a lot for the exceptionally detailed report and the fix! 🙏 The write-up in #624 was a model bug report: clean repro, correct bisect to #592, and a precise root-cause analysis. I reproduced it on The approach is spot on: re-keying Validating |



Summary
Restores the 3.0.0 active-state resolution path that PR #592 inadvertently changed.
Configuration._instance_statesis re-keyed bystate.value(the canonical identifier already used bystates_map) instead ofstate.id(the Python attribute name, which collides across sibling Compounds with same-named children).Closes #624.
Change
statemachine/statemachine.py:211—_build_configurationwritesinstance_states[state.value] = ist.statemachine/configuration.py:87—Configuration.statesresolves directly:self._instance_states[v](drops the redundant_states_map[v].idhop).statemachine/configuration.py:36— type hint loosened toMapping[Any, State]to match the establisheddict[Any, State]convention used bystates_map.The
vars(self)[state.id] = istuser-facing attribute shortcut atstatemachine.py:213is untouched — orthogonal concern.Tests
New
TestSiblingCompoundIdCollisionclass intests/test_configuration.py:test_dispatch_to_g1_inner_state_both_engines— the exact Sibling Compound states with same-named inner attributes break in 3.1.0+ (regression bisected to #592) #624 repro, parametrized sync/async viasm_runner.test_dispatch_to_g2_inner_state_both_engines— same shape but in the second compound.test_instance_states_keyed_by_value_no_collision— white-box guard:len(_instance_states) == len(states_map)and key sets match.Full suite: 1472 passed, 144 skipped, 44 xfailed in ~23s. Branch coverage on
statemachine.configurationandstatemachine.statemachinepreserved.Out of scope — deeper issue surfaced during review
Charts that rely on default
value=and reuse Python attribute names across sibling Compounds were never supported, in any version:factory.py:341(cls.states_map[state.value] = state) silently overwrites. The chart constructs successfully butstates_maphas fewer entries than declared states. This predates #592 and is not a regression — it cannot be reached or repaired at the_instance_stateslayer.Tracking as separate follow-ups:
InvalidDefinitioninfactory.add_statewhenstate.valuecollides. Surfaces the constraint instead of silently overwriting. Backward-incompatible (charts that "worked by accident" will refuse to construct); should land in a minor release with notes.state.id(SCXML-style globally-unique ids) — would also make the default-value case work, but touches__repr__, diagrams, and callback name resolution. Larger design discussion.